-
Notifications
You must be signed in to change notification settings - Fork 70
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base/*: fix uncompressed inline child resource with compressed parent #341
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only gave this a somewhat superficial look but seems sane to me!
Updated. |
When desugaring inline/local resource contents, the current rules are: 1. If the input config specifies compression: gzip, we pass through the compression field unchanged and refuse to compress again. This is useful for local resources that are stored precompressed alongside the config. 2. If we decide to compress, we set the compression field to gzip. 3. If we decide not to compress, we do not set the compression field, ostensibly so precompressed resources in child configs can inherit the compression field from the parent. But case 3 doesn't really work. Butane doesn't know that the child resource is precompressed, so is allowed to compress it again, and only the compressibility heuristics (which technically are not contractual) prevent it from doing so and producing invalid output. (There's no way to represent nested compression, so the file would be written to the provisioned system compressed.) And if the parent resource is not formally precompressed, but is actually just auto-compressed by Butane, a change to the parent's contents or to the Butane heuristics could cause the parent to become uncompressed in the future and break the merged config. And case 3 creates a problem for an inline/local child resource overriding an inline/local parent. If Butane opts to compress the parent but not the child, the merged config will have uncompressed contents but inherit "compression: gzip" from the parent, causing a provisioning failure. And if the parent/child contents change over time, this problem may suddenly appear in the future. Fix this by changing rule 3. When desugaring a non-precompressed inline/local resource, always set the compression field to either "" or "gzip". This intuitively makes sense: only we know whether we decided to compress the resource, so we need to say so. This involves an API change to MakeDataURL(). We now return a pointer to the selected compression value, or nil if we're passing through the compression value from the input (case 1). In the latter case, the caller should record a 1:1 translation for the compression field, rather than attributing its value to the inline/local input field. Fixes #332.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall. Do we need to surface this somehow also in the Ignition spec docs for the Ignition-level version of the config merge issue?
OK right, while this seems like a useful fix I don't think it will actually help the MCO today, see openshift/machine-config-operator#3126 (comment) |
We describe the field merging semantics in Ignition operator notes, so I don't think so? The issue here was that Butane wasn't taking those semantics into account. |
See coreos/butane#341 This obviates coreos/ignition#1329 which we are carrying currently.
Filed coreos/ignition#1357 for better config merging docs, including an explicit callout of overriding the |
When desugaring inline/local resource contents, the current rules are:
compression: gzip
, we pass through thecompression
field unchanged and refuse to compress again. This is useful for local resources that are stored precompressed alongside the config.compression
field togzip
.compression
field, ostensibly so precompressed resources in child configs can inherit thecompression
field from the parent.But case 3 doesn't really work. Butane doesn't know that the child resource is precompressed, so is allowed to compress it again, and only the compressibility heuristics (which technically are not contractual) prevent it from doing so and producing invalid output. (There's no way to represent nested compression, so the file would be written to the provisioned system compressed.) And if the parent resource is not formally precompressed, but is actually just auto-compressed by Butane, a change to the parent's contents or to the Butane heuristics could cause the parent to become uncompressed in the future and break the merged config.
And case 3 creates a problem for an inline/local child resource overriding an inline/local parent. If Butane opts to compress the parent but not the child, the merged config will have uncompressed contents but inherit
compression: gzip
from the parent, causing a provisioning failure. And if the parent/child contents change over time, this problem may suddenly appear in the future.Fix this by changing rule 3. When desugaring a non-precompressed inline/local resource, always set the
compression
field to either "" orgzip
. This intuitively makes sense: only we know whether we decided to compress the resource, so we need to say so.This involves an API change to
MakeDataURL()
. We now return a pointer to the selectedcompression
value, ornil
if we're passing through thecompression
value from the input (case 1). In the latter case, the caller should record a 1:1 translation for thecompression
field, rather than attributing its value to theinline
/local
input field.Fixes #332.